-
Notifications
You must be signed in to change notification settings - Fork 0
Automated Test: improve-two-factor-authentication-features #361
Automated Test: improve-two-factor-authentication-features #361
Conversation
Co-authored-by: Peer Richelsen <peeroke@gmail.com>
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive two-factor authentication backup codes feature. It adds new components for backup code input and display, extends the 2FA setup flow to generate and present backup codes, enables backup code-based authentication during login and disable operations, updates the backend logic for validation, adds database schema changes, and extends localization with backup code-related strings. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client as Web Client
participant Server as Backend API
participant DB as Database
participant Auth as NextAuth
rect rgba(100, 150, 200, 0.5)
Note over User,DB: Setup 2FA & Generate Backup Codes
User->>Client: Enable Two-Factor Auth
Client->>Server: POST /setup (password)
Server->>Server: Generate 10 random backup codes
Server->>Server: Encrypt backup codes with CALENDSO_ENCRYPTION_KEY
Server->>DB: Store encrypted secret, keyUri, backupCodes
Server->>Client: Return secret, keyUri, dataUri, backupCodes
Client->>Client: Display backup codes in grid
User->>Client: Download/Copy backup codes
end
rect rgba(150, 200, 100, 0.5)
Note over User,Auth: Login with Backup Code (Lost Access)
User->>Client: Click "Lost access" on 2FA screen
User->>Client: Enter backup code (XXXXX-XXXXX format)
Client->>Auth: POST /callback/credentials (email, password, backupCode)
Auth->>Server: Credentials provider authorize callback
Server->>DB: Query user with backupCodes field
DB->>Server: Return user record with encrypted backupCodes
Server->>Server: Decrypt backupCodes using CALENDSO_ENCRYPTION_KEY
Server->>Server: Normalize and match provided code
alt Code matches
Server->>Server: Mark code as consumed (nullify in array)
Server->>Server: Re-encrypt remaining codes
Server->>DB: Update user backupCodes
Server->>Auth: Return authenticated user
Auth->>Client: Login successful
else Code incorrect or missing
Server->>Auth: Throw IncorrectBackupCode or MissingBackupCodes error
Auth->>Client: Display error message
end
end
rect rgba(200, 150, 100, 0.5)
Note over User,DB: Disable 2FA with Backup Code
User->>Client: Open Disable 2FA Modal
Client->>Client: Show TwoFactor input
User->>Client: Click "Lost access" button
Client->>Client: Switch to BackupCode input
User->>Client: Enter password + backup code
Client->>Server: POST /disable (password, backupCode)
Server->>DB: Query user with backupCodes
DB->>Server: Return user encrypted backupCodes
Server->>Server: Decrypt and validate backup code
alt Code valid
Server->>DB: Set twoFactorEnabled=false, backupCodes=null, twoFactorSecret=null
Server->>Client: Success response
Client->>Client: Close modal, reset form
else Code invalid
Server->>Client: Return IncorrectBackupCode error
Client->>Client: Display error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@apps/web/components/auth/BackupCode.tsx`:
- Line 7: The component in this file is misnamed: change the default exported
function name from TwoFactor to BackupCode so the component identity matches
imports (e.g., BackupCode used in login.tsx) and avoids collision with the other
TwoFactor component; update the function declaration/export to "BackupCode" (and
any internal references like props typing or displayName if present) so the
default export and component name are consistent.
In `@apps/web/components/settings/EnableTwoFactorModal.tsx`:
- Line 64: The state backupCodes is initialized with an empty array which
TypeScript infers as never[]; update the useState call in EnableTwoFactorModal
(the backupCodes / setBackupCodes declaration) to include an explicit type
parameter (e.g., useState<string[]>([]) or useState<YourBackupCodeType[]>([]))
so setBackupCodes(body.backupCodes) and mapping over backupCodes have the
correct type; if a BackupCode interface/type exists, use that type instead of
string.
- Around line 273-282: The onClick handler for the Button that calls
navigator.clipboard.writeText(backupCodes.map(formatBackupCode).join("\n")) can
throw and needs error handling: wrap the clipboard call in a try/catch (or use
await navigator.clipboard.writeText(...).catch(...)) inside the onClick for the
Button in EnableTwoFactorModal, and on failure call showToast with an error
message (e.g., t("backup_codes_copy_failed"), "error") while preserving
e.preventDefault(); keep using formatBackupCode and backupCodes as before.
In `@apps/web/pages/api/auth/two-factor/totp/disable.ts`:
- Around line 49-52: The code throws an Error when
process.env.CALENDSO_ENCRYPTION_KEY is missing (the if block checking
CALENDSO_ENCRYPTION_KEY) which causes an unhandled exception; replace the throw
with a proper HTTP response (e.g., return res.status(500).json({ error:
ErrorCode.InternalServerError, message: "Missing encryption key" })) so it
matches the other error branches that use res.status(...).json(...), and apply
the same change to the analogous checks in the TOTP path (the other
CALENDSO_ENCRYPTION_KEY guards around the TOTP handling) to ensure consistent
response-based error handling instead of throwing.
- Around line 46-67: Wrap the JSON.parse(symmetricDecrypt(user.backupCodes,
process.env.CALENDSO_ENCRYPTION_KEY)) call in a try-catch to handle
decryption/parse failures (catch and return a 400 response with an appropriate
ErrorCode), then validate the parsed value is an array using
Array.isArray(backupCodes) before calling backupCodes.indexOf(...); if it's not
an array return a 400 error as well. Ensure you reference user.backupCodes,
symmetricDecrypt, JSON.parse, backupCodes, and req.body.backupCode in the
change.
In `@apps/web/playwright/login.2fa.e2e.ts`:
- Around line 50-53: The current assertion uses page.locator(...).isChecked() (a
Promise<boolean>) wrapped in expect(...).toBeTruthy(), which always passes;
replace it with an awaited Playwright matcher on the locator itself — e.g.,
await expect(page.locator(`[data-testid=two-factor-switch]`)).toBeChecked() or
mirror the working pattern using the attribute check await
expect(page.locator(`[data-testid=two-factor-switch][data-state="checked"]`)).toBeVisible();
ensure you remove the isChecked() call and use the locator-based expect so the
assertion evaluates the resolved state.
In `@packages/features/auth/lib/next-auth-options.ts`:
- Around line 139-141: Wrap the JSON.parse call that parses decrypted backup
codes in a try-catch around the symmetricDecrypt(user.backupCodes,
process.env.CALENDSO_ENCRYPTION_KEY) result (the variable backupCodes) so that
malformed or corrupted JSON does not throw an unhandled exception; on parse
failure, handle the error by returning a controlled error response or throwing a
specific, user-facing error (rather than letting a raw exception bubble up) so
callers of the next-auth-options logic can respond gracefully.
- Around line 131-157: The backupCode branch (condition user.twoFactorEnabled &&
credentials.backupCode) allows bypassing password verification because the
earlier check uses !credentials.totpCode; ensure password verification always
runs when user.password exists by moving or duplicating the password check so it
is executed regardless of totpCode/backupCode presence. Concretely, in
next-auth-options.ts ensure the logic that verifies user.password against
credentials.password is invoked before entering the backup code block (or add
the same password verification inside the backup code branch), then proceed to
decrypt user.backupCodes via symmetricDecrypt, validate and null out the used
code, and re-encrypt with symmetricEncrypt before calling prisma.user.update;
keep existing error throws (ErrorCode.IncorrectBackupCode, MissingBackupCodes,
InternalServerError) intact.
In `@packages/ui/components/form/inputs/Input.tsx`:
- Around line 47-51: The password visibility toggle button in Input.tsx is
currently unreachable by keyboard because it uses tabIndex={-1}; change this to
be keyboard-focusable (remove tabIndex or set tabIndex={0}) so the button (the
element with onClick={() => toggleIsPasswordVisible()}) is operable via Tab, or
if the removal was intentional, add a concise code comment on the button
explaining the accessibility decision and implement an alternative keyboard
activation (e.g., handle a key binding on the input that calls
toggleIsPasswordVisible) so keyboard-only users can toggle the password.
🧹 Nitpick comments (6)
apps/web/pages/api/auth/two-factor/totp/setup.ts (1)
68-68: Backup codes are persisted before 2FA is actually enabled.This mirrors the existing pattern for
twoFactorSecret(also stored at setup, enabled later). If the user abandons setup, bothbackupCodesandtwoFactorSecretlinger in the DB. Not a new problem, but worth noting — a cleanup mechanism for abandoned setups would be beneficial.apps/web/components/settings/TwoFactorAuthAPI.ts (1)
22-25: Consider makingbackupCodeoptional.The
backupCodeparameter is always required by the type signature, but callers will typically only provide it when the user opts for backup code authentication. Making it optional would better express the intent and avoid callers needing to pass an empty string.Proposed change
- async disable(password: string, code: string, backupCode: string) { + async disable(password: string, code: string, backupCode?: string) {apps/web/pages/api/auth/two-factor/totp/disable.ts (1)
48-69: RedundanttwoFactorEnabledchecks.Both
user.twoFactorEnabledchecks at lines 48 and 69 are redundant since line 36 already returns early when!user.twoFactorEnabled. This makes the conditional structure harder to follow than necessary.Simplified structure
- // if user has 2fa and using backup code - if (user.twoFactorEnabled && req.body.backupCode) { + // if using backup code + if (req.body.backupCode) { ... - // if user has 2fa and NOT using backup code, try totp - } else if (user.twoFactorEnabled) { + // NOT using backup code, try totp + } else {apps/web/pages/auth/login.tsx (1)
221-221: Nested ternary is hard to read.This triple-nested ternary on a single line is difficult to parse. Consider extracting to a helper or using explicit conditionals.
More readable alternative
- {twoFactorRequired ? !twoFactorLostAccess ? <TwoFactor center /> : <BackupCode center /> : null} + {twoFactorRequired && ( + !twoFactorLostAccess ? <TwoFactor center /> : <BackupCode center /> + )}apps/web/components/settings/EnableTwoFactorModal.tsx (1)
71-75:resetStatedoesn't clean up blob URL or reset backup code state.When the modal is closed or cancelled, the blob URL created at line 99 is not revoked. While this is minor (it'll be garbage collected on unmount), explicitly revoking it and clearing
backupCodes/backupCodesUrlinresetStatewould be cleaner.Proposed enhancement
const resetState = () => { setPassword(""); setErrorMessage(null); setStep(SetupStep.ConfirmPassword); + if (backupCodesUrl) URL.revokeObjectURL(backupCodesUrl); + setBackupCodes([]); + setBackupCodesUrl(""); };apps/web/components/settings/DisableTwoFactorModal.tsx (1)
29-41: Consider providingdefaultValuestouseForm.The form is initialized without
defaultValues, so all fields (password,totpCode,backupCode) start asundefined. WhileresetFormsets them to"", fields that were never rendered (e.g.,backupCodewhen the user stays in TOTP mode) could be submitted asundefinedto the API.♻️ Suggested improvement
- const form = useForm<DisableTwoFactorValues>(); + const form = useForm<DisableTwoFactorValues>({ + defaultValues: { password: "", totpCode: "", backupCode: "" }, + });
| import { useLocale } from "@calcom/lib/hooks/useLocale"; | ||
| import { Label, TextField } from "@calcom/ui"; | ||
|
|
||
| export default function TwoFactor({ center = true }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Misleading component name: TwoFactor should be BackupCode.
This component is exported as default but named TwoFactor, while it's imported as BackupCode elsewhere (e.g., login.tsx). Naming it BackupCode would eliminate confusion with the actual TwoFactor component in @components/auth/TwoFactor.tsx.
Proposed fix
-export default function TwoFactor({ center = true }) {
+export default function BackupCode({ center = true }) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export default function TwoFactor({ center = true }) { | |
| export default function BackupCode({ center = true }) { |
🤖 Prompt for AI Agents
In `@apps/web/components/auth/BackupCode.tsx` at line 7, The component in this
file is misnamed: change the default exported function name from TwoFactor to
BackupCode so the component identity matches imports (e.g., BackupCode used in
login.tsx) and avoids collision with the other TwoFactor component; update the
function declaration/export to "BackupCode" (and any internal references like
props typing or displayName if present) so the default export and component name
are consistent.
| [SetupStep.DisplayQrCode]: t("2fa_scan_image_or_use_code"), | ||
| [SetupStep.EnterTotpCode]: t("2fa_enter_six_digit_code"), | ||
| }; | ||
| const [step, setStep] = useState(SetupStep.ConfirmPassword); | ||
| const [password, setPassword] = useState(""); | ||
| const [backupCodes, setBackupCodes] = useState([]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useState([]) infers never[] — add type annotation.
Without an explicit type, TypeScript infers never[] for the initial empty array. This will cause type errors when calling setBackupCodes(body.backupCodes) and when mapping over the array.
Proposed fix
- const [backupCodes, setBackupCodes] = useState([]);
+ const [backupCodes, setBackupCodes] = useState<string[]>([]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [backupCodes, setBackupCodes] = useState([]); | |
| const [backupCodes, setBackupCodes] = useState<string[]>([]); |
🤖 Prompt for AI Agents
In `@apps/web/components/settings/EnableTwoFactorModal.tsx` at line 64, The state
backupCodes is initialized with an empty array which TypeScript infers as
never[]; update the useState call in EnableTwoFactorModal (the backupCodes /
setBackupCodes declaration) to include an explicit type parameter (e.g.,
useState<string[]>([]) or useState<YourBackupCodeType[]>([])) so
setBackupCodes(body.backupCodes) and mapping over backupCodes have the correct
type; if a BackupCode interface/type exists, use that type instead of string.
| <Button | ||
| color="secondary" | ||
| data-testid="backup-codes-copy" | ||
| onClick={(e) => { | ||
| e.preventDefault(); | ||
| navigator.clipboard.writeText(backupCodes.map(formatBackupCode).join("\n")); | ||
| showToast(t("backup_codes_copied"), "success"); | ||
| }}> | ||
| {t("copy")} | ||
| </Button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
navigator.clipboard.writeText can throw — add error handling.
The clipboard API may fail in non-secure contexts or if permission is denied, causing an unhandled promise rejection. Wrap in try-catch and show an error toast on failure.
Proposed fix
onClick={(e) => {
e.preventDefault();
- navigator.clipboard.writeText(backupCodes.map(formatBackupCode).join("\n"));
- showToast(t("backup_codes_copied"), "success");
+ navigator.clipboard.writeText(backupCodes.map(formatBackupCode).join("\n")).then(
+ () => showToast(t("backup_codes_copied"), "success"),
+ () => showToast(t("something_went_wrong"), "error")
+ );
}}>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Button | |
| color="secondary" | |
| data-testid="backup-codes-copy" | |
| onClick={(e) => { | |
| e.preventDefault(); | |
| navigator.clipboard.writeText(backupCodes.map(formatBackupCode).join("\n")); | |
| showToast(t("backup_codes_copied"), "success"); | |
| }}> | |
| {t("copy")} | |
| </Button> | |
| <Button | |
| color="secondary" | |
| data-testid="backup-codes-copy" | |
| onClick={(e) => { | |
| e.preventDefault(); | |
| navigator.clipboard.writeText(backupCodes.map(formatBackupCode).join("\n")).then( | |
| () => showToast(t("backup_codes_copied"), "success"), | |
| () => showToast(t("something_went_wrong"), "error") | |
| ); | |
| }}> | |
| {t("copy")} | |
| </Button> |
🤖 Prompt for AI Agents
In `@apps/web/components/settings/EnableTwoFactorModal.tsx` around lines 273 -
282, The onClick handler for the Button that calls
navigator.clipboard.writeText(backupCodes.map(formatBackupCode).join("\n")) can
throw and needs error handling: wrap the clipboard call in a try/catch (or use
await navigator.clipboard.writeText(...).catch(...)) inside the onClick for the
Button in EnableTwoFactorModal, and on failure call showToast with an error
message (e.g., t("backup_codes_copy_failed"), "error") while preserving
e.preventDefault(); keep using formatBackupCode and backupCodes as before.
|
|
||
| // if user has 2fa and using backup code | ||
| if (user.twoFactorEnabled && req.body.backupCode) { | ||
| if (!process.env.CALENDSO_ENCRYPTION_KEY) { | ||
| console.error("Missing encryption key; cannot proceed with backup code login."); | ||
| throw new Error(ErrorCode.InternalServerError); | ||
| } | ||
|
|
||
| if (!user.backupCodes) { | ||
| return res.status(400).json({ error: ErrorCode.MissingBackupCodes }); | ||
| } | ||
|
|
||
| const backupCodes = JSON.parse(symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY)); | ||
|
|
||
| // check if user-supplied code matches one | ||
| const index = backupCodes.indexOf(req.body.backupCode.replaceAll("-", "")); | ||
| if (index === -1) { | ||
| return res.status(400).json({ error: ErrorCode.IncorrectBackupCode }); | ||
| } | ||
|
|
||
| // we delete all stored backup codes at the end, no need to do this here | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap JSON.parse(symmetricDecrypt(...)) in try-catch and validate the result.
If decryption fails or the stored value isn't valid JSON, JSON.parse(symmetricDecrypt(...)) will throw an unhandled exception, resulting in a raw 500 error. Additionally, there's no validation that the parsed result is actually an array before calling .indexOf().
Proposed fix
- const backupCodes = JSON.parse(symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY));
+ let backupCodes: string[];
+ try {
+ backupCodes = JSON.parse(symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY));
+ } catch (e) {
+ console.error("Failed to decrypt or parse backup codes for user", session.user.id);
+ return res.status(500).json({ error: ErrorCode.InternalServerError });
+ }
+
+ if (!Array.isArray(backupCodes)) {
+ console.error("Backup codes are not an array for user", session.user.id);
+ return res.status(500).json({ error: ErrorCode.InternalServerError });
+ }🤖 Prompt for AI Agents
In `@apps/web/pages/api/auth/two-factor/totp/disable.ts` around lines 46 - 67,
Wrap the JSON.parse(symmetricDecrypt(user.backupCodes,
process.env.CALENDSO_ENCRYPTION_KEY)) call in a try-catch to handle
decryption/parse failures (catch and return a 400 response with an appropriate
ErrorCode), then validate the parsed value is an array using
Array.isArray(backupCodes) before calling backupCodes.indexOf(...); if it's not
an array return a 400 error as well. Ensure you reference user.backupCodes,
symmetricDecrypt, JSON.parse, backupCodes, and req.body.backupCode in the
change.
| if (!process.env.CALENDSO_ENCRYPTION_KEY) { | ||
| console.error("Missing encryption key; cannot proceed with backup code login."); | ||
| throw new Error(ErrorCode.InternalServerError); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent error handling: throw vs res.status().json().
Line 51 uses throw new Error(...) for a missing encryption key, which will result in an unhandled exception and a generic 500. Other error conditions in this function (e.g., lines 55, 63) use res.status(400).json(...). Prefer returning a proper response for consistency and to avoid unhandled exceptions.
Proposed fix
if (!process.env.CALENDSO_ENCRYPTION_KEY) {
console.error("Missing encryption key; cannot proceed with backup code login.");
- throw new Error(ErrorCode.InternalServerError);
+ return res.status(500).json({ error: ErrorCode.InternalServerError });
}Note: The same pattern exists in the TOTP path (lines 77, 82, 90), which is pre-existing.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!process.env.CALENDSO_ENCRYPTION_KEY) { | |
| console.error("Missing encryption key; cannot proceed with backup code login."); | |
| throw new Error(ErrorCode.InternalServerError); | |
| } | |
| if (!process.env.CALENDSO_ENCRYPTION_KEY) { | |
| console.error("Missing encryption key; cannot proceed with backup code login."); | |
| return res.status(500).json({ error: ErrorCode.InternalServerError }); | |
| } |
🤖 Prompt for AI Agents
In `@apps/web/pages/api/auth/two-factor/totp/disable.ts` around lines 49 - 52, The
code throws an Error when process.env.CALENDSO_ENCRYPTION_KEY is missing (the if
block checking CALENDSO_ENCRYPTION_KEY) which causes an unhandled exception;
replace the throw with a proper HTTP response (e.g., return
res.status(500).json({ error: ErrorCode.InternalServerError, message: "Missing
encryption key" })) so it matches the other error branches that use
res.status(...).json(...), and apply the same change to the analogous checks in
the TOTP path (the other CALENDSO_ENCRYPTION_KEY guards around the TOTP
handling) to ensure consistent response-based error handling instead of
throwing.
| // FIXME: this passes even when switch is not checked, compare to test | ||
| // below which checks for data-state="checked" and works as expected | ||
| await page.waitForSelector(`[data-testid=two-factor-switch]`); | ||
| await expect(page.locator(`[data-testid=two-factor-switch]`).isChecked()).toBeTruthy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FIXME is correct — isChecked() assertion is always truthy.
page.locator(...).isChecked() returns a Promise<boolean>. Wrapping a Promise in expect(...).toBeTruthy() always passes because a Promise object is truthy regardless of its resolved value. The second test (line 127) correctly uses await expect(page.locator(\[data-testid=two-factor-switch][data-state="checked"]`)).toBeVisible()` — consider applying the same pattern here.
Proposed fix
- // FIXME: this passes even when switch is not checked, compare to test
- // below which checks for data-state="checked" and works as expected
- await page.waitForSelector(`[data-testid=two-factor-switch]`);
- await expect(page.locator(`[data-testid=two-factor-switch]`).isChecked()).toBeTruthy();
+ await expect(page.locator(`[data-testid=two-factor-switch][data-state="checked"]`)).toBeVisible();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // FIXME: this passes even when switch is not checked, compare to test | |
| // below which checks for data-state="checked" and works as expected | |
| await page.waitForSelector(`[data-testid=two-factor-switch]`); | |
| await expect(page.locator(`[data-testid=two-factor-switch]`).isChecked()).toBeTruthy(); | |
| await expect(page.locator(`[data-testid=two-factor-switch][data-state="checked"]`)).toBeVisible(); |
🤖 Prompt for AI Agents
In `@apps/web/playwright/login.2fa.e2e.ts` around lines 50 - 53, The current
assertion uses page.locator(...).isChecked() (a Promise<boolean>) wrapped in
expect(...).toBeTruthy(), which always passes; replace it with an awaited
Playwright matcher on the locator itself — e.g., await
expect(page.locator(`[data-testid=two-factor-switch]`)).toBeChecked() or mirror
the working pattern using the attribute check await
expect(page.locator(`[data-testid=two-factor-switch][data-state="checked"]`)).toBeVisible();
ensure you remove the isChecked() call and use the locator-based expect so the
assertion evaluates the resolved state.
| if (user.twoFactorEnabled && credentials.backupCode) { | ||
| if (!process.env.CALENDSO_ENCRYPTION_KEY) { | ||
| console.error("Missing encryption key; cannot proceed with backup code login."); | ||
| throw new Error(ErrorCode.InternalServerError); | ||
| } | ||
|
|
||
| if (!user.backupCodes) throw new Error(ErrorCode.MissingBackupCodes); | ||
|
|
||
| const backupCodes = JSON.parse( | ||
| symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY) | ||
| ); | ||
|
|
||
| // check if user-supplied code matches one | ||
| const index = backupCodes.indexOf(credentials.backupCode.replaceAll("-", "")); | ||
| if (index === -1) throw new Error(ErrorCode.IncorrectBackupCode); | ||
|
|
||
| // delete verified backup code and re-encrypt remaining | ||
| backupCodes[index] = null; | ||
| await prisma.user.update({ | ||
| where: { | ||
| id: user.id, | ||
| }, | ||
| data: { | ||
| backupCodes: symmetricEncrypt(JSON.stringify(backupCodes), process.env.CALENDSO_ENCRYPTION_KEY), | ||
| }, | ||
| }); | ||
| } else if (user.twoFactorEnabled) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: Password verification bypass when backupCode is used with a non-empty totpCode.
The password check at Line 121 is gated by !credentials.totpCode. An attacker who knows a valid backup code and the user's email can supply an arbitrary totpCode string alongside the backupCode, skipping the password check entirely:
- Line 121:
user.password && !credentials.totpCode→false(totpCode is present) → password never verified. - Line 131:
user.twoFactorEnabled && credentials.backupCode→ backup code path succeeds without password.
The backup code branch must independently verify the password, or the password check condition should not be skipped when backupCode is provided.
🔒 Proposed fix: ensure password is verified before the backup code path
- if (user.password && !credentials.totpCode) {
+ if (user.password && !credentials.totpCode && !credentials.backupCode) {
if (!user.password) {
throw new Error(ErrorCode.IncorrectEmailPassword);
}
const isCorrectPassword = await verifyPassword(credentials.password, user.password);
if (!isCorrectPassword) {
throw new Error(ErrorCode.IncorrectEmailPassword);
}
}
if (user.twoFactorEnabled && credentials.backupCode) {
+ // Verify password before accepting backup code
+ if (user.password) {
+ const isCorrectPassword = await verifyPassword(credentials.password, user.password);
+ if (!isCorrectPassword) {
+ throw new Error(ErrorCode.IncorrectEmailPassword);
+ }
+ }
+
if (!process.env.CALENDSO_ENCRYPTION_KEY) {Alternatively, a simpler refactor is to always check the password when user.password exists, regardless of other credential fields:
- if (user.password && !credentials.totpCode) {
+ if (user.password && !(credentials.totpCode || credentials.backupCode)) {And add the same password check inside the backup code branch as shown above.
🤖 Prompt for AI Agents
In `@packages/features/auth/lib/next-auth-options.ts` around lines 131 - 157, The
backupCode branch (condition user.twoFactorEnabled && credentials.backupCode)
allows bypassing password verification because the earlier check uses
!credentials.totpCode; ensure password verification always runs when
user.password exists by moving or duplicating the password check so it is
executed regardless of totpCode/backupCode presence. Concretely, in
next-auth-options.ts ensure the logic that verifies user.password against
credentials.password is invoked before entering the backup code block (or add
the same password verification inside the backup code branch), then proceed to
decrypt user.backupCodes via symmetricDecrypt, validate and null out the used
code, and re-encrypt with symmetricEncrypt before calling prisma.user.update;
keep existing error throws (ErrorCode.IncorrectBackupCode, MissingBackupCodes,
InternalServerError) intact.
| const backupCodes = JSON.parse( | ||
| symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY) | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unhandled JSON.parse on decrypted backup codes can crash with a 500.
If backupCodes data is corrupted or decryption produces invalid JSON, JSON.parse will throw an unhandled exception, leaking an internal error to the user. Wrap in try-catch and return a meaningful error.
🛡️ Proposed fix
- const backupCodes = JSON.parse(
- symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY)
- );
+ let backupCodes: (string | null)[];
+ try {
+ backupCodes = JSON.parse(
+ symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY)
+ );
+ } catch {
+ console.error("Failed to decrypt or parse backup codes for user", user.id);
+ throw new Error(ErrorCode.InternalServerError);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const backupCodes = JSON.parse( | |
| symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY) | |
| ); | |
| let backupCodes: (string | null)[]; | |
| try { | |
| backupCodes = JSON.parse( | |
| symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY) | |
| ); | |
| } catch { | |
| console.error("Failed to decrypt or parse backup codes for user", user.id); | |
| throw new Error(ErrorCode.InternalServerError); | |
| } |
🤖 Prompt for AI Agents
In `@packages/features/auth/lib/next-auth-options.ts` around lines 139 - 141, Wrap
the JSON.parse call that parses decrypted backup codes in a try-catch around the
symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY) result
(the variable backupCodes) so that malformed or corrupted JSON does not throw an
unhandled exception; on parse failure, handle the error by returning a
controlled error response or throwing a specific, user-facing error (rather than
letting a raw exception bubble up) so callers of the next-auth-options logic can
respond gracefully.
| <button | ||
| className="text-emphasis h-9" | ||
| tabIndex={-1} | ||
| type="button" | ||
| onClick={() => toggleIsPasswordVisible()}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tabIndex={-1} removes the password toggle from keyboard navigation.
This prevents keyboard-only users from reaching the show/hide password button via Tab. While this is a common pattern to streamline form tab order, it does reduce accessibility — WCAG 2.1 SC 2.1.1 (Keyboard) expects all interactive elements to be operable via keyboard.
If the intent is to keep the toggle out of the main tab flow, consider allowing focus via an alternative keyboard mechanism (e.g., a key binding on the input itself) rather than removing it entirely. If this is a deliberate UX decision for the 2FA flow, a brief code comment explaining the rationale would help future maintainers.
🤖 Prompt for AI Agents
In `@packages/ui/components/form/inputs/Input.tsx` around lines 47 - 51, The
password visibility toggle button in Input.tsx is currently unreachable by
keyboard because it uses tabIndex={-1}; change this to be keyboard-focusable
(remove tabIndex or set tabIndex={0}) so the button (the element with
onClick={() => toggleIsPasswordVisible()}) is operable via Tab, or if the
removal was intentional, add a concise code comment on the button explaining the
accessibility decision and implement an alternative keyboard activation (e.g.,
handle a key binding on the input that calls toggleIsPasswordVisible) so
keyboard-only users can toggle the password.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
Release Notes